Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: initial implementation #1

Merged
merged 13 commits into from
Jan 31, 2024
Merged

feat: initial implementation #1

merged 13 commits into from
Jan 31, 2024

Conversation

amunra
Copy link
Collaborator

@amunra amunra commented Jan 29, 2024

Initial implementation, see README.md for details.

@amunra amunra force-pushed the v0_1_0 branch 4 times, most recently from bd08793 to 2f6fac0 Compare January 29, 2024 11:06
@amunra amunra requested a review from jerrinot January 29, 2024 11:23
README.md Outdated Show resolved Hide resolved
@amunra amunra marked this pull request as ready for review January 29, 2024 13:05
Copy link
Contributor

@jerrinot jerrinot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both the impl and the spec look very nice!

one possible improvement: document FFI. things like: "how to test it". this is what I did:

cmake .
make

This generated a binary test which indeed runs the test. but I can see there is also a run script, which is trying to execute ./build/test. But my binary is in the working dir, not inside build.

this is no biggie, but documenting the blessed steps how to run the test would be nice.

(after writing all of the above, I noticed there is the compile script shows everything. and this script is used on CI. so I guess a sentence in FFI readme will do)

Copy link
Contributor

@jerrinot jerrinot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized it's actually overly restrictive - we do need underscore in keys. right now, the grammar does not make it possible. and indeed, this is failing:

#[test]
fn key_with_underscore() {
    let input = "https::addr=localhost;tls_roots=/etc/ssl/certs/ca-certificates.crt;";
    let config = parse_conf_str(input);
    assert!(!config.is_err());
}

@amunra amunra merged commit be71c4e into main Jan 31, 2024
2 checks passed
@amunra amunra deleted the v0_1_0 branch January 31, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants